First, thanks for diving into this complex issue. Unfortunately I'm seeing the same behavior you describe in #190 even using this code. I will keep digging because I'd like to better understand what's going on here but I likely won't get to it until next week. It's almost certainly a weird interaction with the recent changes to better support international keyboards.

However, I do find the complexity of the key handling algorithm uncomfortably complex, and I think it's largely due to the keyup semantics.

No disagreement here! Unfortunately this same complexity makes it a significant effort and high risk to rewrite.

I'm wondering: how would you feel about simplifying it so that instead of thinking of the keyup (^foo) as an event in the binding sequence, it's more like an annotation that just means "... and wait until foo is released." Which I'm hoping is sufficient for all real use cases.

I think I don't understand this. The only keyup binding that ships in a default install of Atom now is for MRU tab traversal:

  'ctrl-tab': 'pane:show-next-recently-used-item'
  'ctrl-tab ^ctrl': 'pane:move-active-item-to-top-of-stack'
  'ctrl-shift-tab': 'pane:show-previous-recently-used-item'
  'ctrl-shift-tab ^ctrl': 'pane:move-active-item-to-top-of-stack'

So when a user presses ctrl-tab we need to immediately dispatch that command and wait until they release ctrl before dispatching the command that updates the MRU stack. What do you mean by "...and wait until foo is released?"

keyups can only appear at the end of a binding (good: "alt-a ^alt", bad: "alt-a ^alt a")
keyups can only apply to modifier keys (bad: "alt-a ^alt-a")
there can only be one keyup (bad: "alt-shift-a ^alt ^shift") [alternatively we could just convert this to "alt-shift-a ^alt-shift"]
keyups can only specify the release of keys that were pressed earlier in the binding (bad: "alt-a ^ctrl")

I actually experimented with a set of validations very much like this one recently. In the end it was a ton of complexity for dubious gain: it's possible to add all kinds of keybindings that can lead to bizarre behavior within the app. There's not a clear benefit to attempting to prevent this particular subset.

on key down:

immediately fire the first exact match (with no keyups)
for each possible keyup suffix:
  Add the first such binding to a separate list of pendingKeyup bindings

on key up:

find bindings in pendingKeyup pending this key release and fire them.
(If there are multiple modifiers pending release, like `^alt-shift`, all need to be released.)

I believe this is actually an accurate description of the current behavior. Pending keyup matches get stashed in pendingKeyupMatcher. When there's a keyup event it updates its state and returns matches.

I'll stop here for now and see if we can come to a better common understanding via a couple more comment roundtrips. :) Again, I do really appreciate your taking the time to dive into some super gnarly code. More from me next week.

My mistake; the example in #190 failed for me too -- I was testing that the tab-switcher example worked (where the focus moves to another element on keydown). I've added another commit which fixes both.

What do you mean by "...and wait until foo is released?"

The distinction I was trying to make between the current & proposed behavior is we just wait until foo is released, as opposed to treating "^foo" as a full-fledged keystroke that could be surrounded by other keyup and keydown events in the binding. It sounds like you would agree that this is sufficient.

I believe this is actually an accurate description of the current behavior. Pending keyup matches get stashed in pendingKeyupMatcher. When there's a keyup event it updates its state and returns matches.

Thanks for the pointers; I had indeed overlooked the PartialKeyupMatcher bit, so it's a lot closer than I thought. However, I still find it unnecessarily complex that the keyup handling code includes all of the keydown handling, so e.g. the keyups can wind up on the queued keystrokes list, which means they're subject to replaying (which led to the bug(s) here).

What I'm thinking is that for a binding like "alt-a ^alt", everything could happen when alt-a is pressed except the actual command dispatch and emit() call, which is delayed until alt is released. And everything except those two things could be skipped for the keyup event. This would mean the keystrokes are removed from the queued keystrokes on keydown too.

I actually experimented with a set of validations very much like this one recently. In the end it was a ton of complexity for dubious gain: it's possible to add all kinds of keybindings that can lead to bizarre behavior within the app. There's not a clear benefit to attempting to prevent this particular subset.

I can see adding validations as adding complexity, but all I'm proposing is that scenarios outside these cases be considered undefined behavior -- that is, we shouldn't worry about changing the behavior for these unlikely cases in order to simplify things internally. (I actually would favor showing a warning if bindings include anything considered UB, but agree there is a complexity tradeoff.)

There is a legitimate bug here but since we agree this PR doesn't resolve it, I'm going to close it.

@iolsen I believe this PR does solve the current problems. (First paragraph of my last comment: "I've added another commit which fixes both.")

Any thoughts on this fix?

🤦‍♂️ sorry I missed that. I'll take a look!

@iolsen did you get a chance to take a look at this?

@oggy so sorry for the delay. I haven't looked at this yet but should have time in the next few weeks.

This problem is no longer happening as of 1.19 or 1.21-dev.